Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[py] Added Common Tests to Edge CI #14748

Merged
merged 9 commits into from
Nov 15, 2024
Merged

[py] Added Common Tests to Edge CI #14748

merged 9 commits into from
Nov 15, 2024

Conversation

shbenzer
Copy link
Contributor

@shbenzer shbenzer commented Nov 12, 2024 •
edited
Loading

User description

Description

Added "test/selenium/webdriver/common/**/*.py" to edge tests in Bazel.Build

Motivation and Context

Building off @Delta456 's #14723 . Figured since this will add so many tests it might be best to split into a separate pr.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

enhancement, tests

Description

  • Added common Selenium WebDriver tests to the Edge CI test suite in the Bazel build configuration.
  • This change enhances the test coverage for Edge by including tests from the common directory.

Changes walkthrough 📝

Relevant files
Tests
BUILD.bazel
Include common tests in Edge CI test suite                             

py/BUILD.bazel

  • Added common test files to the Edge test suite.
  • Updated the srcs attribute to include common tests.
    		•  +1/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Test Overlap
    Adding all common tests to edge test suite may include tests that are not applicable or compatible with Edge browser. Consider reviewing the common test files to ensure they are all relevant for Edge testing.

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 12, 2024 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use more specific glob patterns to ensure only test files are included in test targets

    Consider using a more specific glob pattern to include only test files, such as
    "test.py" or "test.py", to avoid accidentally including non-test Python files.

    py/BUILD.bazel [503-506]

     srcs = glob([
-    "test/selenium/webdriver/edge/**/*.py",
-    "test/selenium/webdriver/common/**/*.py",
+    "test/selenium/webdriver/edge/**/*_test.py",
+    "test/selenium/webdriver/common/**/*_test.py",
 ]),
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using more specific glob patterns (*_test.py) is a good practice to prevent accidentally including non-test files in test targets, which could lead to build issues or unnecessary file inclusion. This improves build clarity and maintainability.

    		7

    💡 Need additional feedback ? start a PR chat

    @VietND96
    Copy link
    Member 

        py_test_suite(
        name = "common-%s" % browser,
        size = "large",
        srcs = glob(
            [
                "test/selenium/webdriver/common/**/*.py",
                "test/selenium/webdriver/support/**/*.py",
            ],

    @AutomatedTester, I saw this test suite using config from browsers.bzl, should this be duplicated?

    @shbenzer
    Copy link
    Contributor Author

    @VietND96 Merge 1de1b40 introduced errors into //py:test-edge-test/selenium/webdriver/edge/edge_launcher_tests.py and //py:test-edge-test/selenium/webdriver/common/w3c_interaction_tests.py

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Nov 14, 2024 •
    edited
    Loading

    Only the bidi_tests.py had problems in initial commit - those have been marked xfail like safari since problem is on the webdriver side

    @VietND96
    Copy link
    Member

    Yes, I am updating your PR and @Delta456 to merge both

    @VietND96 VietND96 merged commit d55eff3 into SeleniumHQ:trunk Nov 15, 2024
    17 checks passed
    @VietND96
    Copy link
    Member

    VietND96 commented Nov 15, 2024 •
    edited
    Loading

    In set of common tests including BiDi test, when running bazel test //py:test-edge, it doesn't set args --bidi=true, so there were failed tests
    So, the CI keep running 2 command separately
    bazel test //py:common-$browser-bidi - run tests in /common/.py and /support/.py against different browser driver
    bazel test //py:test-$browser - test of particular browser driver
    Also updated README on the list of commands for python tests

    jkim2492 pushed a commit to jkim2492/selenium that referenced this pull request Nov 17, 2024
    @shbenzer @VietND96 @jkim2492
    [py] Added Common Tests to Edge CI (SeleniumHQ#14748)
    7ec713e 
    ---------

Signed-off-by: Viet Nguyen Duc <[email protected]>
Co-authored-by: Viet Nguyen Duc <[email protected]>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @AutomatedTester AutomatedTester AutomatedTester approved these changes

    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 1
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    3 participants
    @shbenzer @VietND96 @AutomatedTester